Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

retrieval: Direct piecereader #6128

Closed
wants to merge 10 commits into from
Closed

retrieval: Direct piecereader #6128

wants to merge 10 commits into from

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Apr 28, 2021

Cherry-picks parts of #5769, and makes them work

Did some testing in a pond devnet with a worker, it seems to work, but needs testing on a real node, and ideally some integration tests

@magik6k magik6k requested a review from arajasek as a code owner April 28, 2021 18:23
Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one question

extern/sector-storage/fr32/readers.go Outdated Show resolved Hide resolved
markets/retrievaladapter/provider.go Outdated Show resolved Hide resolved
extern/sector-storage/stores/remote.go Outdated Show resolved Hide resolved
Copy link
Contributor

@arajasek arajasek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing seems very wrong, and I think I understand what it does. May even work.

@dirkmc dirkmc mentioned this pull request Apr 30, 2021
@aarshkshah1992 aarshkshah1992 mentioned this pull request May 6, 2021
9 tasks
@dirkmc
Copy link
Contributor

dirkmc commented May 10, 2021

When we tried to run this with our miner we got the following error when doing a retrieval:

2021-05-04T18:32:57.675+0300    INFO    markets loggers/loggers.go:30   retrieval provider event        {"name": "ProviderEventCancelComplete", "deal ID": "1620120981223977232", "receiver": "12D3KooWHMGTcNbS64arHyaPoUv6NrZi9V63kuVbnE6jbBjm5EUS", "state": "DealStatusErrored", "message": "failed to unseal piece from sector 647: opening partial file: file '/root/storage_sealing/unsealed/s-t0127896-647' has inconsistent length; has 34359738377 bytes; expected 2057 (9 trailer, 2048 sector data)"}

@magik6k
Copy link
Contributor Author

magik6k commented May 17, 2021

^ likely caused by not setting the seal proof type in the http handler / remote store on either the worker or miner node

@aarshkshah1992
Copy link
Contributor

aarshkshah1992 commented May 17, 2021

@magik6k Thanks for that pointer. I'll grok this PR, factor in your suggestion and take this work to completion as we really need it to fix data retrievals.

Btw, do you have any pointers to existing integration tests that we can use as reference to write integration tests for this feature ?

@magik6k
Copy link
Contributor Author

magik6k commented May 17, 2021

Btw, do you have any pointers to existing integration tests that we can use as reference to write integration tests for this feature ?

Api tests which do retrieval (like this one) should be exercising most of the now code here (I'd run them with coverage to see if that's actually the case).

Testing the remote parts / new http code is a bit more complex as it requires setting up workers in the test framework, which is currently not there, and will probably require refactoring the worker constructor to be usable in tests (cmd/lotus-seal-worker/main.go), possibly by making it use the DI framework (I could help with this - if not, having it be non-DI based for now is also fine, the worker isn't that complicated)

@magik6k
Copy link
Contributor Author

magik6k commented May 17, 2021

(for worker tests, basically we'd need to extract the whole worker thing from the run command function, and make it possible to provide things which are now taken from cctx (miner API, miner store address, temp repo) from a test)

@magik6k
Copy link
Contributor Author

magik6k commented May 17, 2021

I think this is the issue:

I think the correct thing to do here is to just set the proof type in the fsm adapter from info.SectorType (which is the first field set after sectors are created, so it should be set no matter at which stage of the sealing pipeline sectors are)

@magik6k
Copy link
Contributor Author

magik6k commented May 21, 2021

Superseded by #6280

@magik6k magik6k closed this May 21, 2021
@Kubuxu Kubuxu deleted the feat/direct-piecereader branch November 25, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants